HIP/CUDA Gen: Use correct block size bounds in non-tensor dynamic dispatch calculations#1993
HIP/CUDA Gen: Use correct block size bounds in non-tensor dynamic dispatch calculations#1993zatkins-dev wants to merge 3 commits into
Conversation
…ze (instead of constant 512)
…gher order non-tensor elements without fallback to ref
zatkins-dev
changed the title
|
See #1994 for my thoughts on how to automatically catch these failures moving forward. |
|
Also, note that we have to use |
|
Also, should we use a more sophisticated algorithm for the HIP block and grid sizes, like we do for CUDA? It seems like the limitation was in not knowing the optimal minimum grid size and block sizes, which we now have access to. |
|
We need to do profiling here probably, because I think this has serious perf implications? @nbeams and @YohannDudouit know way better than I |
FWIW -- The current code always falls back for Nitsche operators in Ratel, regardless of quadrature/solution order. This is a sizable perf improvement just by virtue of actually supporting gen in those cases. Not sure about general non-tensor, but I imagine this will fix some issues there too. |
|
Good point. I think a combo of profiling and expert opinion probably is still appropriate to make sure we are appropriately slicing the available resources? |
| cuda_data->device_prop.maxThreadsDim[2], cuda_data->device_prop.warpSize, block, &grid)); | ||
| } else { | ||
| CeedInt elems_per_block = CeedIntMin(cuda_data->device_prop.maxThreadsDim[2], CeedIntMax(512 / data->thread_1d, 1)); | ||
| CeedInt elems_per_block = CeedIntMin(cuda_data->device_prop.maxThreadsDim[2], CeedIntMax(max_threads_per_block / data->thread_1d, 1)); |
There was a problem hiding this comment.
We could also leave the cap on 512 by doing this:
| CeedInt elems_per_block = CeedIntMin(cuda_data->device_prop.maxThreadsDim[2], CeedIntMax(max_threads_per_block / data->thread_1d, 1)); | |
| CeedInt elems_per_block = CeedIntMin(cuda_data->device_prop.maxThreadsDim[2], CeedIntMax(CeedIntMin(max_threads_per_block, 512) / data->thread_1d, 1)); |
That would ensure the previous behavior for the standard non-tensor elements while preventing the elems_per_block from being too high for the weird cases.
There was a problem hiding this comment.
That's probably a good safeguard here
|
My recollection: Back when we added the launch bounds to the HIP kernels, it (adding launch bounds) was recommended as best practice by our AMD liaisons for CEED, for performance. And also, an equivalent to But now that something like the CUDA routine is available, I guess the question is whether or not its (hopefully better) guess for occupancy is more important than the launch bounds or not. Of course since we have to compile the kernel first, if we either add launch bounds prematurely or go back and add launch bounds after calling the max occupancy routine, there is a chance it could change the kernel produced in a way that would alter the occupancy calculation. I really have no idea how much the launch bounds matter for recent ROCm and on the latest AMD GPUs, but it would be interesting to check. |
3 participants
Purpose:
Removes hard coded constants for block size bounds with dynamically queried values for both HIP and CUDA gen backend non-tensor kernels.
Also, fixes a bug in the field reuse code in HIP.
Closes:
LLM/GenAI Disclosure:
None.
By submitting this PR, the author certifies to its contents as described by the Developer's Certificate of Origin.
Please follow the Contributing Guidelines for all PRs.